Skip to content

fix(images): name an unread image's bytes in the placeholder - #783

Merged
Birfy merged 2 commits into
mainfrom
fix/779-image-bytes
Oct 22, 2026
Merged

fix(images): name an unread image's bytes in the placeholder#783
Birfy merged 2 commits into
mainfrom
fix/779-image-bytes

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Written-by: Dev

Tip SHA: 865c84e

Fixes #779

What changed

  • core/src/host/images.js: imagePlaceholder() is now the single placeholder for both places an image turns into text, and it names the file when the Bot can open it. attachmentFilePath() resolves the attachment store's host path through the file sandbox's mapping (processPathFromHostPath) and falls back to the local object layout (<home>/attachments/v1/objects/<first two>/<sha256>); either candidate must stat as a real file, and an image id that is not sha256:<64 hex> gets no path at all. imageAsText (Bot chat) and imagesForGroup (group relay) both pass the path through it, and only look it up when the reading failed.
  • core/test/images.test.js: the object layout and its refusal of other ids; a file that exists is named, a directory and a missing file are not; the store path wins when the sandbox maps it and the object layout is the fallback; the three placeholder forms.
  • core/test/session-identity.test.js: the harness takes runtime config and returns its home; with every vision route disabled (textOnlyModels), an unread image names its bytes in a group relay and in a Bot chat alike, no vision request goes out, and an image whose bytes are gone keeps the short placeholder.
  • docs/subsystems/core-host.md: the image subsection its arch-docs ownership asks for (route and cache, and the placeholder rule); core/README.md and README.md: images are read by a model that declares image input (the configured ocrModel first), and the placeholder names the file when no model can read it. No configuration is changed by this PR: cordis.patch.yml stays as it is.
  • patches/ds-bot/: src/host/images.js, test/images.test.js and test/session-identity.test.js joined 0001-core-fixes in topics.json, and the patch was regenerated from core/.

What was wrong

A picture no model can read reaches the Bot as [Image "…" (…): you cannot see images and it could not be read right now. Ask the user what it shows if it matters.], while the bytes sit in the attachment store on the same machine — on this box /data/dsh/attachments/v1/objects/bd/bdc77c70…, 213569 bytes, with the log line could not read image sha256:bdc77c70…: no vision model is available. Nothing in the placeholder says where the file is, so a Bot that could open it with its own file tools asks the user to describe the picture instead.

What I ran

At this tip (base d83d762d, Node 24.10.0 + DSH 0.2.0-rc.2 from npm, TZ=UTC):

  • core: node --import ./test/harness-resolve.mjs --test test/images.test.js test/session-identity.test.js → 12/12.
  • node .github/scripts/changelog.mjs check → 38 fragments, all well formed.
  • The patch series: patches/ds-bot/files/ + all patches applied to ds-bot/ reproduces core/ (114 patched files, MATCH 331 files).

Red without the change, each cut alone at this tip:

  • the path dropped from imagePlaceholder() → 10/12: red on the placeholder names the file, its size and its type; without a path it stays as it was and an unread image names its bytes in a group and in a chat alike — the two assertions that carry the fix.
  • the stat check replaced by "any string is a path" → 9/12: red on a file the Bot can open is named, and one that is not there is never invented, the attachment store's path wins when the file sandbox can map it (its missing-file half) and an unread image whose bytes are gone keeps the placeholder short.

Not run on this box: npm run lint (no oxlint here), scripts/ds-bot-patches.mjs check and arch-docs (both need git), smoke and Desktop. CI runs them at this tip.

Risk

T3 (core/src/host/**). Narrow: the placeholder text grows by one sentence and only when a readable file exists; the existing reading, caching and offload paths are unchanged, and a failed lookup returns the placeholder exactly as before.

@Birfy
fix(images): name an unread image's bytes in the placeholder
69c4b06 
An image that no model can read becomes a short placeholder, and that was all the
Bot got: it could not open the picture, and the content-addressed bytes sat on
disk unmentioned. Reproduced with a picture in Bot chat on a tree with no vision
model: the request carried "[Image ...: it could not be read right now.]" and the
warning "could not read image sha256:...: no vision model is available", while
/data/.../attachments/v1/objects/bd/bdc... was 213 KB on the same machine.

`imagePlaceholder()` is now the one placeholder for both places an image becomes
text (a Bot chat through `imageAsText`, the group relay through `imagesForGroup`),
and it names the file when the Bot can open it. `attachmentFilePath()` resolves it
the way the rest of the core reaches files: the attachment store's host path goes
through the file sandbox's mapping (`processPathFromHostPath`), and the local
object layout (`<home>/attachments/v1/objects/<first two>/<sha256>`) is the
fallback when the store or the mapping is absent. Either way `stat` must succeed
and the path must be a file: a placeholder that points at nothing is worse than no
path, and an image id that is not content-addressed gets none.

Pinned by #779: the object layout and its refusal of non-sha256 ids; the file
exists / is a directory / is missing cases; the store-and-mapping path and the
fallback; the three placeholder forms; and, in the Session harness with every
vision route disabled, that an unread image names its bytes both in a group relay
and in a Bot chat, and stays short when the bytes are gone. Drop the path from the
placeholder and images+session-identity is 10/12 with the two path assertions red;
drop the `stat` check and it is 9/12 with the three "must exist" assertions red.

With the fix: 12/12 in those two files, and `patches/ds-bot` regenerates to the
same `core/` (applycheck: 114 patched files, 331 compared, MATCH).

Part of #779.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
@github-actions github-actions Bot added the risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff label Oct 22, 2026
@Birfy
docs(core-host): document the image placeholder and the readable path
865c84e 
`core/src/host/images.js` is an owned path of the `core-host` subsystem, so the arch-docs
gate fails a change to it without an update to `docs/subsystems/core-host.md`. This adds the
missing subsection: which model reads an image (`ocrRoute`, `ocrModel`, `textOnlyModels`), that
the reading is cached in `<home>/ocr.json`, and the rule the placeholder follows — one
placeholder for a Bot chat and the group relay, the file named only when the Bot's own tools can
open it (the store's `imageHostPath` through `processPathFromHostPath`, else the local object
layout, either way `stat`-checked), nothing copied into the working folder.

No behaviour change; the code and its tests are unchanged at this tip.

Part of #779.
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 865c84e
Written-by: Reviewer2

Ran (head 865c84ef checked out over main 177d0ffa; Node 22.23.3, DSH_HARNESS=/opt/flybot TZ=UTC):

  • cd core && node --import ./test/harness-resolve.mjs --test test/images.test.js test/session-identity.test.js → 12/12 at the tip
  • probe 1, the point of the change: made imagePlaceholder never emit the path (const where = '') → 2 red / 10 pass: #779: the placeholder names the file, its size and its type; without a path it stays as it was and an unread image names its bytes in a group and in a chat alike; restored → 12/12
  • probe 2: dropped the stat guard in readable() so any candidate path is returned → 3 red / 9 pass: the "never invented" case, the store-mapping case and an unread image whose bytes are gone keeps the placeholder short; restored → 12/12
  • probe 3: kept the store's host path but skipped the sandbox mapping (const mapped = hostPath) → 1 red / 11 pass, only #779: the attachment store's path wins when the file sandbox can map it; restored → 12/12
  • cd core && node --import ./test/harness-resolve.mjs --test --test-concurrency=4 "test/*.test.js" → 1274 tests / 1271 pass / 3 fail, the same three as pristine main on this machine (empty diff passes, composer-label, read-page)

What I checked:

  • CI at 865c84e: 9/9 completed and green (tests-ok, package, plugins (tree|social|timeline), ubuntu Node 24, merge-gate, reviewed tip, risk tier); mergeable: clean; no hold; no other conclusion at this tip
  • the scope is the placeholder half only: cordis.patch.yml is not in the diff, no model declaration, defaultInput/input or ocrModel is touched, so no half-true vision route is created. A model that declares image input still does the reading; this only improves the text that stands in for a reading that did not happen
  • one placeholder, two callers: imagePlaceholder() is used by imageAsText (Bot chat) and imagesForGroup (group relay), and the group form drops the "you cannot see images" half; the case asserts both paths on the same text
  • the path is never a guess: the attachment store's host path must map through fs.processPathFromHostPath (the same seam dsh-spill-policy uses), the local object layout is only the fallback, stat must report a regular file either way, and an id that is not sha256:<64 hex> gets no path at all — probe 2 shows the "never invent a path" half is pinned, probe 3 shows the mapping is pinned
  • the lookup happens only when the reading failed, so a readable image pays no stat and no placeholder change
  • docs/subsystems/core-host.md gains the image subsection its arch-docs ownership asked for, core/README.md and README.md describe the model requirement, and the patch series carries src/host/images.js plus both test files through topics.json

Not run: a real vision model (this host has none — that is #779's other half, deliberately out of this diff), so the "read succeeds" branch is exercised only by the existing cases, not by this one. node scripts/ds-bot-patches.mjs check cannot run here (no git); package runs it on CI and is green.

Non-blocking:

  • attachmentObjectPath hard-codes <home>/attachments/v1/objects/<xx>/<sha>; a future store layout change would silently stop naming files, and the fallback would quietly disappear rather than fail. The store path through imageHostPath is the part that follows the store's own API.
  • the placeholder now carries media type, dimensions and byte count as well; that is more than Chat images cannot be read: the host has no vision model, and the placeholder hides where the bytes are #779 asked for and it is what the Bot needs to decide whether to open the file, but it is a second change inside the same sentence.

@github-actions github-actions Bot added the pass label Oct 22, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit dde0748 Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the fix/779-image-bytes branch October 22, 2026 21:50
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Birfy Birfy

Labels

pass risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Chat images cannot be read: the host has no vision model, and the placeholder hides where the bytes are

1 participant

@Birfy